r? @blyxyas

rustbot has assigned @blyxyas.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Lintcheck changes for abab6c7

This comment will be updated if you push new changes

Hello. The implementation of this change turned out to be almost suspiciously easy. I tried to be thorough with testing. Happy to add more cases (lifetimes, etc).

I browsed through the new lintcheck added lines. They seem to be fine. There are a few changed lines, but they seem unrelated to this PR? Not sure.

Probably need doc changes here or there too. Will do if changes are okay.

Hi Nick!
We're currently in a feature freeze until September 18, so I'll mark this as blocked. Even though it's a very simple case, the reason for the feature freeze isn't that adding features, is hard, is that maintaining them is harder. Sorry for the inconvenience.

@rustbot note Feature-freeze
@rustbot blocked

Addressed @ada4a 's comments. Updated Clippy sites flagged by lint. Eagerly awaiting feature thaw 🐈

I'm not sure if we want to actually go forward with this. Like, using Self in an enum seems kinda weird and I'm not sure if all that conventional. We'll discuss it the next meeting, you can join if you want.

They're hosted on our channel on Zulip =^w^=

Recursive enums are not all that common, but when an enum is recursive Self is very nice to have. This pattern is already used in Clippy itself:

#[derive(Clone, Copy)]
enum NormalizedPat<'a> {
    Wild,
    Never,
    Struct(Option<DefId>, &'a [(Symbol, Self)]),  // <--
    Tuple(Option<DefId>, &'a [Self]),             // <--
    Or(&'a [Self]),                               // <--
    Path(Option<DefId>),
    LitStr(Symbol),
    LitBytes(ByteSymbol),
    LitInt(u128),
    LitBool(bool),
    Range(PatRange),
    /// A slice pattern. If the second value is `None`, then this matches an exact size. Otherwise
    /// the first value contains everything before the `..` wildcard pattern, and the second value
    /// contains everything afterwards. Note that either side, or both sides, may contain zero
    /// patterns.
    Slice(&'a [Self], Option<&'a [Self]>),        // <--
    /// A placeholder for a pattern that wasn't well formed in some way.
    Err(ErrorGuaranteed),
}

Use of Self makes it obvious that this enum is recursive, and it also makes it obvious exactly which fields are recursive. It also reduces noisy lifetime repetition.

If Self in enum or struct seems "unconventional", that may just be because it's a language feature that has been poorly publicized. I only became aware of it myself when I had to add lifetimes to a recursive struct, which without Self is an annoying task. (I was additionally annoyed that Clippy had not already alerted me to this even though I have use_self enabled, hence my attempt to add the feature.)

Part of the purpose of a linter like Clippy is to encourage use of language features. "I see you have a recursive struct / enum, did you know that you can use Self instead of explicitly repeating the name?" use_self already does a good job of encouraging use of Self in impl blocks. It could also do the same here.

Hello. I would like to get this merged soon. Sounds like the result of the meeting is to add this new check, but include a config option to disable it.

What should be the name / description of the option?

Recent change #15773 illustrates the benefits of Self for recursive objects. Constant was modified to remove a lifetime. Because it uses explicit name repetition, all of its recursive fields had to modified as well. With Self, those fields could have been left alone.

Hi Nick, sorry for the delay. I thought I had responded one of the last 3 times I revisited this pull request this week.
In the meeting indeed we concluded that it should be guarded by a configuration option. While there isn't a convention for configuration names, the name should probably be recursive-self-in-struct, with the description being something like

Whether the type itself in a struct or enum should be replaced with `Self` when encountering recursive types

@rustbot ready

@rustbot note remove Feature-freeze

recursive-self-in-struct

Since it applies to enums as well, maybe it should be -in-adt (which is probably the most correct, but not the most understandable) or -in-item (which is more understandable, but technically less correct, as impl blocks are Items as well)?

Since it applies to enums as well, maybe it should be -in-adt

I've thought about it, but decided to just mention structs (extending it to enums in the description) because I don't think anybody outside of the compiler space knows what an ADT actually is.

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

I've thought about it, but decided to just mention structs (extending it to enums in the description) because I don't think anybody outside of the compiler space knows what an ADT actually is.

recursive-self-in-types would be a more appropriate name. recursive-self-in-type-definitions if you want to be extra wordy.